Skip to content

feat(change_request): redesign Create Group & Add Member CRs + review-page fixes (#876 #871)#242

Open
emjay0921 wants to merge 27 commits into
19.0from
feat/876-create-group-cr-redesign
Open

feat(change_request): redesign Create Group & Add Member CRs + review-page fixes (#876 #871)#242
emjay0921 wants to merge 27 commits into
19.0from
feat/876-create-group-cr-redesign

Conversation

@emjay0921

Copy link
Copy Markdown
Contributor

Why is this change needed?

QA review of spp_cr_types_advanced returned findings on the Create Group (#876) and Add Member (#871) change-request flows. The main issue: the review page showed only counts for relational data (phones, bank accounts, ID documents, members) and omitted several defined fields (email, area, location, member demographics).

This branch consolidates both tickets (they share spp_change_request_v2, so feat/871 was merged into feat/876 to stop cross-merging).

How was the change implemented?

A generic render contract on each strategy's preview(), rendered by _generate_review_comparison_html():

  • _tables — one2many fields render as their own tables (Phone Numbers, Bank Accounts, ID Documents, Existing Members).
  • _sections — entities that are full records (new group members) render as a labelled detail block each, with all fields + that member's own phones.
  • Helpers _render_data_tables() / _render_data_sections() are generic and reused by both Create Group and Add Member.
  • #876: Create Group review shows the above + adds email/area/location.
  • #871: Add Member review shows the added individual's full field set (even when empty → "—") + the tables. The head-of-household replacement feature was reverted and deferred to #1006.

New unit tests

  • test_create_group_strategy.py: test_preview_one2many_as_tables_and_scalars, test_preview_members_table_and_sections, test_review_comparison_html_renders_tables (+ updated test_preview).
  • test_add_member_strategy.py: test_preview_shows_fields_and_tables.

Unit tests executed by the author

spp_change_request_v2 full suite: 0 failed, 0 error of 324 tests. Pre-commit clean.

How to test manually

Create a Create Group and an Add Member CR with phones/bank/ID docs and members, advance to the Review stage → Proposed Changes, and confirm the data shows as tables / detail blocks (not counts), all fields appear, and the Add Member head-replacement toggle is gone. (See the QA guides on #876 / #871.)

Related links

#876, #871 (parent: Review [spp_cr_types_advanced]); head-of-household handling deferred to #1006

emjay0921 added 24 commits June 2, 2026 15:51
Replace the flat head-of-household fields on the Create Group detail with
member-list sub-tables and per-type configuration:

- Detail: drop head_individual_id / create_new_head / head_* fields; add
  phone_line_ids, bank_line_ids, id_doc_line_ids, member_existing_ids,
  member_new_ids sub-tables, plus area_id and latitude/longitude.
- CR type: add allow_empty_members and requires_head booleans so each
  group-creating type ships its own rules; apply strategy validates both.
- Wizard: new spp.cr.detail.create_group.member.wizard handles add/edit
  for existing + new members in one transient model.
- Strategy: rewrite apply() into _validate / _create_group / _attach_*
  helpers; preview() reports per-sub-table counts and head label.
- View: notebook 'Members' page with per-mode alert blocks and primary
  buttons; member lists are wizard-only (create=0 edit=0).
- Type registration: create_group sets is_requires_registrant=False —
  the CR creates the registrant, so the picker is hidden in the create
  wizard. create_wizard.py renders a tailored "creates a new ..." hint.
- MIS demo generator: split head_name into given/family and emit a
  member_new_ids row with the 'head' role.
- Security: ACLs for the 5 new sub-models + wizard.
- Tests: rewrite test_create_group_strategy.py around the new schema
  (empty / existing-head / new-head / mixed / name-required); update
  two E2E call sites in test_e2e_workflows.py.
Replace the flat 7-field Add Member detail with the full spec layout from
OP#871, reusing the per-line sub-models introduced by OP#876.

- Detail: add middle_name; demographics block (is_approximate_birthdate,
  age compute, birth_place, occupation_id, civil_status_id, income);
  area_id + address + email; latitude/longitude; one-to-many phone /
  bank / id_doc lines; membership_type_id (role); related mirror
  type_requires_head; computed roles_available and existing_membership_ids
  (read-only context table). member_name is now a stored compute of
  "FAMILY, GIVEN MIDDLE".
- Shared sub-models: make detail_id nullable on
  spp.cr.detail.create_group.{phone,bank,id_doc}, add a parallel
  add_member_detail_id FK, and an exactly-one-parent constraint so the
  same row shape can attach to either detail.
- Strategy: rewrite into _validate / _create_individual /
  _attach_phones|banks|id_docs / _demote_existing_head_if_needed /
  _create_membership. When the new member is added with the 'head' role,
  any existing head on the group is demoted automatically. Names are
  required; if the CR type sets requires_head, role assignment is too.
- View: full rewrite matching spec sections (Name, Demographics, Contact,
  Phone Numbers, Location, Financial Information, Identity Document,
  Members) with the review-stage banner "The following individual is to
  be added:" and a warning banner when requires_head is set.
- MIS demo generator: write membership_type_id instead of the removed
  relationship_id; drop the now-computed member_name from the payload.
- Tests: rewrite test_add_member_strategy.py against the new schema
  (basic create, name compute including middle name, age compute,
  validation paths, head demotion, sub-record attach); fix the four
  add_member call sites in test_e2e_workflows.py.

Stacks on top of feat/876-create-group-cr-redesign.
- Expose Allow Empty Groups / Requires Head of Household on the Change
  Request Type configuration form
- Collect the group address as a single field matching the registry
  (was split into Address Line 1/2, City, State, Postal Code, Country)
- Enforce one Head of Household at the member-row level so the rule also
  holds when rows are added through the member wizard
- Capture the full registry profile (names, demographics, contact) for a
  new individual in the Add New Individual modal
- Show "The following group is to be added" above the review table
- Collect the member address as a single field matching the registry
  (was split into Address Line 1/2, City, State, Postal Code, Country)
- Make the existing-members table truly read-only — rows can no longer
  be opened and edited from the change request form
- Show "The following individual is to be added" above the review table
- Map approximate-birthdate to the registry individual on apply

Requires Head of Household is now configurable on the Change Request
Type form (merged from #876).
The Add New Individual modal now collects multiple phone numbers in an
editable list (matching the group's phone list) instead of a single
field. On apply, the numbers are folded (primary first) into the new
individual's single header phone field. The create-group phone row
model gained a member_new parent so the rows attach to the new member.
…mber-cr-redesign

# Conflicts:
#	spp_change_request_v2/details/create_group.py
…r (#876)

Editing a new in-group individual that already had phone numbers raised
"Exactly one parent must be set on a phone-number row". The wizard
rebuilt the list with a clear (5) command, which only nulls the phone
row's inverse FK instead of deleting it, momentarily orphaning the rows.
Use delete (2) commands so the old rows are removed cleanly.
…i-parenting (#876)

The exactly-one-parent constraint also rejected zero-parent rows, which
Odoo can produce transiently while rewriting a one2many — surfacing a
confusing "Exactly one parent must be set" error to users editing a new
member's phone list. Only reject a row genuinely linked to more than one
record.
…mber-cr-redesign

# Conflicts:
#	spp_change_request_v2/details/create_group.py
…#876)

A new in-group individual's phone numbers are all folded into the
partner's single phone field, so there is no header-vs-other distinction
— the Primary toggle was meaningless. Remove it from the modal and join
the numbers in entry order.
…hone rows (#876)

Root cause of the phone-row 'parent' errors: the Add New Individual
wizard is opened with a default_detail_id context (for the member row),
and the phone sub-model also has a detail_id field, so that default was
applied to the new phone rows — giving them two parents (detail_id +
member_new_id). Force detail_id=False when building the phone rows.
Adds a regression test that opens the wizard with the leaking context.
…hone rows (#876)

Root cause of the phone-row 'parent' errors: the Add New Individual
wizard is opened with a default_detail_id context (for the member row),
and the phone sub-model also has a detail_id field, so that default was
applied to the new phone rows — giving them two parents (detail_id +
member_new_id). Force detail_id=False when building the phone rows.
Adds a regression test that opens the wizard with the leaking context.
…ply (#876)

A new individual's captured phone numbers were only folded into the
partner's single header phone field, not created as spp.phone.number
records — so they didn't appear in the registry's Phone Numbers list.
Attach the phone records on apply, the same way the group's phones are
attached.
…ember (#871)

When the target group already has a head, the Add Member form now shows
an info banner naming the current head and a 'Replace current Head of
Household' toggle. Replacement is no longer silent/automatic: giving the
new member the Head role while a head exists is blocked unless the toggle
is enabled; when enabled, applying demotes the current head and makes the
new member the head.
… is Head (#871)

The banner and Replace toggle now appear only when the picked Role is
Head AND the group already has a head. The toggle is purely a
confirmation gate: the picked Role determines the member's role, so a
stale toggle with a non-head role no longer forces head or demotes the
current head.
…ice (#871)

Drop the 'Replace current Head of Household' boolean. When the new member
is given the Head role and the group already has a head, the form now
shows a warning that applying will make this member the head and remove
the role from the current head; the replacement then happens
automatically on apply (the CR still passes through approval first).
…ew (#876)

The Create Group CR review page (review_comparison_html) showed only
counts for the one2many fields and omitted several group fields. Surface
the actual data via a generic render contract on preview():

- _tables: one2many fields render as their own tables - Phone Numbers,
  Bank Accounts, ID Documents, and Existing Members (Name / Role).
- _sections: new members render as a labelled detail block each, showing
  the full captured individual (role, DOB, gender, civil status,
  occupation, birth place, income, area, address, email) plus that
  member's own phone numbers.
- Add the previously-missing scalar fields: email, area, location.
- Drop the phone/bank/id_doc/member count keys.

_generate_review_comparison_html() renders _tables and _sections after
the action summary; _render_data_tables() / _render_data_sections() are
generic so other CR strategies can surface the same way.
…ber (#871)

The explicit head-of-household replacement feature (commits 908cedc,
1612822, a8c9b05) is deferred to #1006 ("make it configurable to
prevent groups without head of household"), which is on hold. Removing
it here per QA so #871 ships the Add Member review-page fix without it.
… (#871)

The Add Member CR review page showed only counts for phones/bank/ID docs
and omitted the added individual's details. Surface the full data via the
generic _tables contract (shared with Create Group):

- All individual fields are shown even when empty (name, role, date of
  birth, gender, civil status, occupation, birth place, income, area,
  address, email, location).
- Phone Numbers / Bank Accounts / ID Documents render as their own tables.
- Drop the phone/bank/id_doc count keys.

The head-of-household replacement feature was reverted separately and
deferred to #1006.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request redesigns the 'Add Member' and 'Create New Group' Change Request workflows to capture a comprehensive set of profile fields and sub-records (phones, banks, ID documents) using a new wizard. Feedback focuses on resolving a bug where zero values are incorrectly treated as empty in previews, using timezone-aware Odoo methods for age calculations, and batching database inserts for better performance when attaching sub-records.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +1214 to +1215
for label, value in field_rows:
display = html_escape(value) if value else '<span class="text-muted">—</span>'

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The current logic treats 0 or 0.0 as falsy, which will display a placeholder instead of the actual value (e.g., for zero income or age). Additionally, passing non-string values directly to html_escape can raise a TypeError. Converting the value to a string and explicitly checking against None, False, and empty strings is safer and more robust.

Suggested change
for label, value in field_rows:
display = html_escape(value) if value else '<span class="text-muted">—</span>'
for label, value in field_rows:
display = html_escape(str(value)) if value not in (None, False, '') else '<span class="text-muted">—</span>'


@api.depends("birthdate")
def _compute_age(self):
today = date.today()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using date.today() uses the server's local timezone, which can lead to incorrect age calculations for users in different timezones. In Odoo, it is best practice to use fields.Date.context_today(self) to get the current date in the user's timezone.

Suggested change
today = date.today()
today = fields.Date.context_today(self)


@api.depends("birthdate")
def _compute_age(self):
today = date.today()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using date.today() uses the server's local timezone, which can lead to incorrect age calculations for users in different timezones. In Odoo, it is best practice to use fields.Date.context_today(self) to get the current date in the user's timezone.

Suggested change
today = date.today()
today = fields.Date.context_today(self)

Comment on lines +189 to +224
def _attach_phones(self, detail, individual):
SppPhone = self.env["spp.phone.number"]
for line in detail.phone_line_ids:
SppPhone.create(
{
"partner_id": individual.id,
"phone_no": line.phone_no,
"country_id": line.country_id.id if line.country_id else False,
"date_collected": fields.Date.today(),
}
)

def _attach_banks(self, detail, individual):
Bank = self.env["res.partner.bank"]
for line in detail.bank_line_ids:
vals = {
"partner_id": individual.id,
"acc_number": line.acc_number,
}
if line.acc_holder_name:
vals["acc_holder_name"] = line.acc_holder_name
if line.bank_id:
vals["bank_id"] = line.bank_id.id
Bank.create(vals)

def _attach_id_docs(self, detail, individual):
RegId = self.env["spp.registry.id"]
for line in detail.id_doc_line_ids:
RegId.create(
{
"partner_id": individual.id,
"id_type_id": line.id_type_id.id,
"value": line.value,
"expiry_date": line.expiry_date,
}
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Creating records inside a loop (SppPhone.create, Bank.create, RegId.create) is a performance bottleneck in Odoo because it triggers multiple database inserts. Accumulating the values in a list and calling create once with the list of dictionaries is much more efficient.

    def _attach_phones(self, detail, individual):
        SppPhone = self.env["spp.phone.number"]
        phone_vals = [
            {
                "partner_id": individual.id,
                "phone_no": line.phone_no,
                "country_id": line.country_id.id if line.country_id else False,
                "date_collected": fields.Date.today(),
            }
            for line in detail.phone_line_ids
        ]
        if phone_vals:
            SppPhone.create(phone_vals)

    def _attach_banks(self, detail, individual):
        Bank = self.env["res.partner.bank"]
        bank_vals = []
        for line in detail.bank_line_ids:
            vals = {
                "partner_id": individual.id,
                "acc_number": line.acc_number,
            }
            if line.acc_holder_name:
                vals["acc_holder_name"] = line.acc_holder_name
            if line.bank_id:
                vals["bank_id"] = line.bank_id.id
            bank_vals.append(vals)
        if bank_vals:
            Bank.create(bank_vals)

    def _attach_id_docs(self, detail, individual):
        RegId = self.env["spp.registry.id"]
        doc_vals = [
            {
                "partner_id": individual.id,
                "id_type_id": line.id_type_id.id,
                "value": line.value,
                "expiry_date": line.expiry_date,
            }
            for line in detail.id_doc_line_ids
        ]
        if doc_vals:
            RegId.create(doc_vals)

Comment on lines +233 to +270
def _attach_phones(self, phone_lines, partner):
"""Create spp.phone.number records (the registry's Phone Numbers list)
on ``partner`` from the given phone rows."""
SppPhone = self.env["spp.phone.number"]
for line in phone_lines:
SppPhone.create(
{
"partner_id": partner.id,
"phone_no": line.phone_no,
"country_id": line.country_id.id if line.country_id else False,
"date_collected": fields.Date.today(),
}
)

def _attach_banks(self, detail, group):
Bank = self.env["res.partner.bank"]
for line in detail.bank_line_ids:
vals = {
"partner_id": group.id,
"acc_number": line.acc_number,
}
if line.acc_holder_name:
vals["acc_holder_name"] = line.acc_holder_name
if line.bank_id:
vals["bank_id"] = line.bank_id.id
Bank.create(vals)

def _attach_id_docs(self, detail, group):
RegId = self.env["spp.registry.id"]
for line in detail.id_doc_line_ids:
RegId.create(
{
"partner_id": group.id,
"id_type_id": line.id_type_id.id,
"value": line.value,
"expiry_date": line.expiry_date,
}
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Creating records inside a loop (SppPhone.create, Bank.create, RegId.create) is a performance bottleneck in Odoo because it triggers multiple database inserts. Accumulating the values in a list and calling create once with the list of dictionaries is much more efficient.

    def _attach_phones(self, phone_lines, partner):
        """Create spp.phone.number records (the registry's Phone Numbers list)
        on partner from the given phone rows."""
        SppPhone = self.env["spp.phone.number"]
        phone_vals = [
            {
                "partner_id": partner.id,
                "phone_no": line.phone_no,
                "country_id": line.country_id.id if line.country_id else False,
                "date_collected": fields.Date.today(),
            }
            for line in phone_lines
        ]
        if phone_vals:
            SppPhone.create(phone_vals)

    def _attach_banks(self, detail, group):
        Bank = self.env["res.partner.bank"]
        bank_vals = []
        for line in detail.bank_line_ids:
            vals = {
                "partner_id": group.id,
                "acc_number": line.acc_number,
            }
            if line.acc_holder_name:
                vals["acc_holder_name"] = line.acc_holder_name
            if line.bank_id:
                vals["bank_id"] = line.bank_id.id
            bank_vals.append(vals)
        if bank_vals:
            Bank.create(bank_vals)

    def _attach_id_docs(self, detail, group):
        RegId = self.env["spp.registry.id"]
        doc_vals = [
            {
                "partner_id": group.id,
                "id_type_id": line.id_type_id.id,
                "value": line.value,
                "expiry_date": line.expiry_date,
            }
            for line in detail.id_doc_line_ids
        ]
        if doc_vals:
            RegId.create(doc_vals)

@codecov

codecov Bot commented Jun 16, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.96398% with 76 lines in your changes missing coverage. Please review.
✅ Project coverage is 71.53%. Comparing base (c660896) to head (adbcc7f).
⚠️ Report is 106 commits behind head on 19.0.

Files with missing lines Patch % Lines
spp_change_request_v2/details/create_group.py 85.25% 23 Missing ⚠️
spp_change_request_v2/wizards/create_wizard.py 7.14% 13 Missing ⚠️
spp_change_request_v2/details/add_member.py 76.74% 10 Missing ⚠️
spp_change_request_v2/strategies/create_group.py 93.80% 7 Missing ⚠️
spp_mis_demo_v2/models/mis_demo_generator.py 0.00% 7 Missing ⚠️
spp_change_request_v2/models/change_request.py 89.83% 6 Missing ⚠️
spp_change_request_v2/strategies/change_hoh.py 82.14% 5 Missing ⚠️
...pp_change_request_v2/models/change_request_type.py 87.50% 2 Missing ⚠️
...e_request_v2/wizards/create_group_member_wizard.py 97.43% 2 Missing ⚠️
spp_change_request_v2/strategies/add_member.py 96.29% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             19.0     #242      +/-   ##
==========================================
+ Coverage   71.50%   71.53%   +0.02%     
==========================================
  Files        1001     1004       +3     
  Lines       58626    59883    +1257     
==========================================
+ Hits        41921    42836     +915     
- Misses      16705    17047     +342     
Flag Coverage Δ
spp_api_v2_change_request 66.85% <ø> (ø)
spp_base_common 90.26% <ø> (ø)
spp_change_request_v2 77.46% <88.02%> (+2.00%) ⬆️
spp_cr_type_assign_program 91.17% <ø> (ø)
spp_cr_types_advanced 0.00% <ø> (ø)
spp_cr_types_base 0.00% <ø> (ø)
spp_dci_demo 19.87% <ø> (-49.36%) ⬇️
spp_farmer_registry_cr 61.15% <ø> (+0.05%) ⬆️
spp_farmer_registry_demo 54.01% <ø> (+0.62%) ⬆️
spp_programs 65.09% <ø> (+0.25%) ⬆️
spp_registry 86.83% <ø> (ø)
spp_security 66.66% <ø> (ø)
spp_starter_farmer_registry 0.00% <ø> (ø)
spp_starter_social_registry 0.00% <ø> (ø)
spp_starter_sp_mis 81.25% <ø> (ø)
spp_studio_change_requests 84.67% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
spp_change_request_v2/__manifest__.py 0.00% <ø> (ø)
spp_change_request_v2/details/change_hoh.py 100.00% <100.00%> (+35.71%) ⬆️
spp_change_request_v2/wizards/__init__.py 100.00% <100.00%> (ø)
spp_change_request_v2/strategies/add_member.py 93.87% <96.29%> (+2.44%) ⬆️
...pp_change_request_v2/models/change_request_type.py 69.41% <87.50%> (+1.87%) ⬆️
...e_request_v2/wizards/create_group_member_wizard.py 97.43% <97.43%> (ø)
spp_change_request_v2/strategies/change_hoh.py 84.61% <82.14%> (-4.86%) ⬇️
spp_change_request_v2/models/change_request.py 84.66% <89.83%> (+0.30%) ⬆️
spp_change_request_v2/strategies/create_group.py 93.18% <93.80%> (-0.94%) ⬇️
spp_mis_demo_v2/models/mis_demo_generator.py 67.40% <0.00%> (+1.49%) ⬆️
... and 3 more

... and 30 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…ady has one (#871)

A group can have at most one Head of Household, so the Head role is now
removed from the Role options when the target group already has an active
head. Implemented as a computed allowed_role_ids driving the Role field
domain (UI restriction only; head-of-household configurability stays with
#1006).
Replace the single new-head dropdown with an editable members-with-roles
table: one role line is seeded per active group member, and the member
assigned the Head role becomes the new head (a constraint blocks two heads).
Make the current head non-clickable, drop the unused Effective Date field,
and remove the Voluntary Transfer reason.

Add reason-driven required documents: a per-reason document config on the
CR type (shown only for types whose detail exposes a reason) drives the
request's required documents via _get_effective_required_document_ids().

The review preview now renders the household, reason, remarks and a members
table (Name / Current Role / New Role).
Per the updated #871 spec, the Add Member CR first page now searches for an
existing individual registrant instead of building a new individual from
scratch.

- detail model: replace the create-new field set (names, demographics, phone/
  bank/ID lines) with an individual_id picker scoped by a computed domain to
  existing individuals not already in the group; keep the Role picker and the
  'no Head when the group already has one' restriction.
- strategy: apply() creates a membership for the selected individual; preview()
  reads the individual's fields and shows 'The following individual is to be
  added to the group'.
- view: first page is a member search + role.

Also update the MIS demo generator so its Add Member and Change Head of
Household change requests match the redesigned detail contracts (register the
individual then set individual_id; rebuild the change-of-head role lines).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant